Combine radar sweeps into a logical radar volume. #307
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
==========================================
+ Coverage 93.52% 93.58% +0.06%
==========================================
Files 27 27
Lines 5572 5613 +41
==========================================
+ Hits 5211 5253 +42
+ Misses 361 360 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
egouden
force-pushed
the
create_volume
branch
3 times, most recently
from
2cec870 to
8415f6d
Compare
|
I added the option to select sweeps by angle. This is useful if you want to discard a calibration scan (i.e. 90 degree). |
|
I found inconsistencies in the global variable I think the string is better because it is consistent with the variable name and you can use it directly for accessing the sweeps. I guess this has no negative impact on the performance. |
egouden
force-pushed
the
create_volume
branch
2 times, most recently
from
4c71b4b to
41ab76e
Compare
|
|
egouden
force-pushed
the
create_volume
branch
from
41ab76e to
5fcb5c4
Compare
egouden
force-pushed
the
create_volume
branch
from
5fcb5c4 to
4ebd2a6
Compare
kmuehlbauer
left a comment
kmuehlbauer
left a comment
There was a problem hiding this comment.
@egouden This is a nice addition. I've added a couple of comments.
| # Step 4: Sort by time | ||
| sweep_entries.sort(key=lambda x: x[1]) | ||
|
|
||
| if not sweep_entries: |
There was a problem hiding this comment.
Wouldn't it make more sense to return an empty volume, instead of raising here?
There was a problem hiding this comment.
This is a philosophical question. Returning an empty volume is misleading in my opinion. It could break downstream with less context. I let you decide what is best for consistency across the package.
There was a problem hiding this comment.
What is an empty volume exactly?
There was a problem hiding this comment.
An empty DataTree? A volume missing any sweeps. Just the remaining metadata. But you made a point here. Skip my idea and raise as intended. We can iterate on that later, if the necessity arises.
| time_coverage_start = datetime.datetime.fromisoformat(time_coverage_start) | ||
| time_coverage_end = "2023-04-19T06:55:00Z" | ||
| time_coverage_end = datetime.datetime.fromisoformat(time_coverage_end) | ||
| with pytest.raises(ValueError, match="No sweeps remain after filtering."): |
There was a problem hiding this comment.
See comment below. I'd rather check here for empty volume.
| if not sweep_entries: | ||
| raise ValueError("No sweeps remain after filtering.") | ||
|
|
||
| # Step 5: Prepare root metadata |
There was a problem hiding this comment.
I'm wondering if it would be possible to do this without the deep copy?
2 participants
This is useful for stacking single sweeps or creating a new volume logic based on time.